-
Notifications
You must be signed in to change notification settings - Fork 275
Feature: Merge Expr and GenExpr
#1114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
c13b346 to
8719b91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR merges the Expr and GenExpr classes into a unified expression system. The refactoring replaces the previous dual-system (polynomial Expr and general GenExpr) with a single hierarchy based on Expr, using specialized subclasses like PolynomialExpr, SumExpr, ProdExpr, and various function expressions (ExpExpr, LogExpr, etc.).
Key changes:
- Unified expression representation with
childrenreplacingterms Variableclass no longer inherits fromExpr- Simplified expression tree structure with improved type system
- Refactored constraint creation methods to use the new expression system
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pyscipopt/scip.pyi | Updated Variable class to remove inheritance from Expr |
| src/pyscipopt/scip.pxi | Modified Variable class structure and added operator overloading methods to delegate to expression system; refactored constraint creation methods to use children instead of terms |
| src/pyscipopt/scip.pxd | Updated Variable class declaration to remove Expr inheritance |
| src/pyscipopt/propagator.pxi | Simplified variable creation by removing unnecessary temporary variable |
| src/pyscipopt/expr.pxi | Complete rewrite of expression system replacing Expr/GenExpr duality with unified class hierarchy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3e0359d to
da49cca
Compare
|
Finish the base feature and function. And something needs to be done:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pyscipopt/expr.pxi
Outdated
| raise TypeError("expr must be an Expr instance") | ||
| if lhs is None and rhs is None: | ||
| raise ValueError( | ||
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is grammatically incorrect. It should be "Ranged ExprCons (with both lhs and rhs) isn't supported" or "Ranged ExprCons (with both lhs and rhs) is not supported" instead of "doesn't supported".
| "Ranged ExprCons (with both lhs and rhs) doesn't supported" | |
| "Ranged ExprCons (with both lhs and rhs) is not supported" |
|
|
||
|
|
||
| class ExprCons: | ||
| """Constraints with a polynomial expressions and lower/upper bounds.""" |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring says "Constraints with a polynomial expressions" but should say "Constraints with polynomial expressions" (without "a" before "polynomial"). Also note that ExprCons can now handle non-polynomial expressions too (like exp, log, etc.), so the docstring is inaccurate and should be updated to reflect that it handles general expressions.
| """Constraints with a polynomial expressions and lower/upper bounds.""" | |
| """Constraints with general expressions and lower/upper bounds.""" |
|
Thanks for your work @Zeroto521 !! This is something that should be very very well tested before merging. We're also in the middle of a SCIP meeting, so I can bring this up with the guys and we can quickly take a high level look. |
Updated test cases in test_term.py to create explicit Model instances before adding variables. Added assertion for string representation in test_mul for improved test coverage.
Introduced a pytest fixture to create and share a Model, variable, and Term instance across tests. This reduces code duplication and improves test maintainability.
Replaces dictionary comprehension with dict.fromkeys for initializing the parent class in ProdExpr. This change simplifies the code and maintains the same functionality.
Simplifies the type conversion logic in UnaryExpr.to_subclass by removing the explicit conversion of Number to ConstExpr and adjusting the order of type checks.
Introduces a __bool__ method to the Expr class to allow direct truthiness checks. Refactors existing checks for self.children to use the new __bool__ method, improving code readability and consistency.
Refines addition and multiplication logic in Expr, PolynomialExpr, and ProdExpr to better handle cases involving constants 0 and 1. This improves efficiency and correctness by avoiding unnecessary object creation and ensuring proper simplification of expressions.
Replaces usage of the 'terms' variable with 'children' to improve clarity and consistency when accessing constraint expression children in Model methods. No functional changes were made.
Changed the parameter name from 'idx' to 'key' in the Term.__getitem__ method for consistency and clarity.
Implemented the __iter__ method for the Term class to allow iteration over its variables. Updated internal usage to leverage the new iterator.
Changed the _hash attribute in the Term class to be readonly and updated the __eq__ method to check for instance type before comparing hashes. This improves type safety and encapsulation.
Replace TypeError checks with inequality assertions when comparing Term instances to non-Term objects. This reflects updated behavior where such comparisons return False instead of raising an exception.
Refined the __hash__ implementations to include type information and use _children instead of children. This ensures more robust and type-aware hashing for expression objects.
Explicitly cast values to float and Expr types in ExprCons methods to ensure type correctness and avoid potential type errors. This affects normalization and comparison operator overloads.
Replaces the UnaryOperatorMixin with a new ExprLike base class to unify the interface for expressions and variables. Refactors Expr, Variable, and related classes to use this new structure, moving operator overloads and common logic to ExprLike. Direct instantiation of Expr and Variable is now disallowed, and Variable now maintains an internal PolynomialExpr view for expression operations. Updates type checks and internal helpers to support the new design, improving maintainability and extensibility.
Replaces _is_const checks with explicit type checks for ConstExpr throughout expression operations for clarity and correctness. Introduces a _reset_hash helper to standardize hash invalidation, replacing direct assignments to _hash. Simplifies and corrects logic in several arithmetic and comparison methods, and updates __repr__ for UnaryExpr to improve output consistency.
Changed setObjective and chgReoptObjective to accept ExprLike instead of Expr, updated parameter types and default values for clarity, and removed an unnecessary assertion in setObjective. These changes improve type flexibility and documentation accuracy.
Simplifies the __mul__ method in PolynomialExpr by removing redundant checks and streamlining the multiplication logic. Also updates casting from Cython-style to Python-style in several places for consistency.
Refactored the matrix class hierarchy by renaming MatrixBase to MatrixExprLike in both matrix.pxi and scip.pxi. Updated all relevant class inheritances to improve code clarity and consistency.
Moved the conversion of 'other' to an Expr earlier in the __mul__ method to improve code clarity and ensure type checks use the converted value.
Updated the __hash__ and ptr methods in the Variable class to return size_t instead of Py_hash_t for consistency and type correctness.
Removed __array_priority__ from ExprLike and set MatrixExprLike's __array_priority__ to 100. This change ensures consistent behavior when interacting with NumPy ufuncs and resolves potential priority conflicts.
Replaces repeated (Number, Variable, Expr) type checks with centralized EXPR_OP_TYPES and NUMBER_TYPES tuples. This improves maintainability and consistency in type checking throughout the expression classes.
Type hints were removed from the __hash__ and ptr methods in the Variable class to improve compatibility and simplify the code.
Refactored Expr and related classes to use the public attribute 'children' instead of the internal '_children'. Updated all references and property definitions accordingly for consistency and clarity.
Updated references from cons.expr._children to cons.expr.children in the Model class to use the correct attribute for accessing constraint expression children. This change improves code clarity and ensures compatibility with the current expression API.
Improves detection and conversion of single-term polynomials by introducing _is_single_poly and _to_poly helpers. Removes redundant __add__ override in PolynomialExpr and streamlines sum logic in Expr to ensure correct type conversion for single polynomials. Updates __repr__ in UnaryExpr for consistency with new helpers.
Changed the first parameter of _expr_cmp from 'self' to 'expr' to improve clarity and consistency, updating all internal references accordingly.
Moved the _to_dict function from the Expr class to a standalone cdef function for improved clarity and reusability. Updated all internal calls to use the new function signature and removed the method declaration from the class definition.
Refines handling of exponentiation for Expr and PolynomialExpr classes, including special cases for exponents 0 and 1, and optimizes polynomial multiplication by using a dictionary for intermediate results. These changes improve correctness and efficiency in mathematical operations.
Introduces a new _normalize function to handle normalization and coefficient scaling of expression children. Replaces multiple dictionary comprehensions with calls to _normalize for improved code reuse and clarity.
Refactored the _to_dict function to use C-level dict access for improved efficiency and to handle zero coefficients correctly. This change avoids unnecessary copying and ensures more accurate updates when combining expression children.
Refactors the __mul__ method in PolynomialExpr to use PyDict_Next for iterating over dictionary items, improving performance and reducing Python overhead. This change also skips zero coefficients and accumulates products directly in the result dictionary.
Replaces the use of next(iter(expr.children)) with PyDict_Next for more direct and efficient access to the first child key in Expr. Raises StopIteration if Expr has no children.
Refactored the __mul__ method in the Term class to efficiently merge variable tuples while maintaining order based on hash values. Introduced a new _extend helper function for extending lists from tuple slices, improving performance and code clarity.
Replaces direct tuple item comparison with explicit Variable extraction and identity check in the Term class equality method. This ensures correct behavior when comparing Term objects.
Removed unnecessary checks for coefficient equal to 1 in multiplication logic and updated normalization to directly copy children when coefficient is 1. This streamlines expression handling and avoids redundant copying.
Deleted the unused internal _extend function to clean up the codebase and improve maintainability.
Deleted custom implementations of exp, log, sqrt, sin, and cos functions, as well as related helper code. These functions are now directly assigned from numpy, simplifying the code and reducing redundancy.
Prevents direct instantiation of ExprCons by raising NotImplementedError in __init__, and introduces a private _expr_cons factory function for internal creation. Updates all internal usages to use _expr_cons, improving encapsulation and guiding users to use comparison operators for constraint creation.
Refactors __pow__ and __rpow__ methods to handle edge cases such as zero and negative exponents or bases more robustly. Adds explicit error handling for invalid operations and clarifies type checks for constant expressions.
Replaces explicit checks for unary numpy ufuncs with a mapping (UNARY_MAP) from ufuncs to method names, simplifying and centralizing the dispatch logic for unary operations in ExprLike.
Close to #1074. This is a breaking change. We can release the 5.7.0 version first.